Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send range with textDocument/hover #45

Closed
wants to merge 1 commit into from
Closed

Conversation

ayoub-benali
Copy link
Collaborator

@ayoub-benali ayoub-benali commented Nov 14, 2021

This is needed to handle scalameta/metals#3060
It should be handle by LSP package once microsoft/language-server-protocol#377
is resolved

Peek 2021-11-14 21-54

@rwols @rchl I duplicated some code that is in hover.py. If you think this behavior change should in https://github.com/sublimelsp/LSP let me know.

Todo:

  • Update readme to mention the required keybinding

closes #44

This is needed to handle scalameta/metals#3060
It should be handle by LSP package once microsoft/language-server-protocol#377
is resolved
@rchl
Copy link
Collaborator

rchl commented Nov 15, 2021

I don't mind this being done here but I guess it doesn't handle the range case when hovering the file with mouse.

I think I wouldn't mind allowing the AbstractPlugin an ability to define special experimental flags that would change the behavior of LSP.

For example something like an additional API method:

def get_experimental_flags(self): dict:
  return {}

That the plugin could override and return something like { 'hover_supports_range': True } which would enable special logic for that specific session.

But before adding something like that, @rwols should also state his opinion.

Copy link

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much better than the workaround in VS Code!

LGTM from overall code perspective, but I do not know enough about the Sublime API to see specific possible issues.

ayoub-benali added a commit to ayoub-benali/LSP that referenced this pull request Nov 15, 2021
This an alternative solution to scalameta/metals-sublime#45
The current implementation is just a workaround until microsoft/language-server-protocol#377 is part of the LSP spec
ayoub-benali added a commit to ayoub-benali/LSP that referenced this pull request Nov 15, 2021
This an alternative solution to scalameta/metals-sublime#45
The current implementation is just a workaround until microsoft/language-server-protocol#377 is part of the LSP spec
ayoub-benali added a commit to ayoub-benali/LSP that referenced this pull request Nov 15, 2021
This a better alternative to scalameta/metals-sublime#45 and this implementation is just a workaround until microsoft/language-server-protocol#377 is part of the LSP spec
@ayoub-benali
Copy link
Collaborator Author

The downside of this approach is that it creates a new command that very similar to the base lsp_hover which might be confusing. I created sublimelsp/LSP#1898 which should provide a better experience.

@ayoub-benali
Copy link
Collaborator Author

Will try to handle this in https://github.com/sublimelsp/LSP rather adding a custom new command

@ayoub-benali ayoub-benali deleted the add-hover-range branch November 16, 2021 09:45
@ckipp01
Copy link
Member

ckipp01 commented Nov 16, 2021

Will try to handle this in https://github.com/sublimelsp/LSP rather adding a custom new command

Again, sorry for confusing this convo and the other one 😆 I need to pay more attention to what repo I'm on. But I'd actually recommend just keeping the implementation in here and not adding it to the core. It's very metals specific, so I totally get the hesitation on the Sublime side to add that, especially because of the way we implemented it.

@rchl
Copy link
Collaborator

rchl commented Nov 16, 2021

Implementation would be very easy in LSP but a proper way to do that would be for the metals server to announce the experimental capability for that feature in the capabilities that it returns from the initialize request. This is a standard practice for implementing experimental functionalities like this one. Once that is done, the LSP can enable such feature easily, based on the server capability.

@ckipp01
Copy link
Member

ckipp01 commented Nov 16, 2021

a proper way to do that would be for the metals server to announce the experimental capability for that feature in the capabilities that it returns from the initialize request

We can totally do this. @ayoub-benali feel free to shoot in a PR for this. You should be able to add the experimental stuff here https://github.com/scalameta/metals/blob/07a66ff10af4b63ee660e1ca432646a051268217/metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala#L786-L833

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Type Annotation on Code Selection
4 participants